feat(web): configurable hotkeys for mobile UI synced from desktop settings#882
feat(web): configurable hotkeys for mobile UI synced from desktop settings#882pedramamini merged 3 commits intoRunMaestro:rcfrom
Conversation
📝 WalkthroughWalkthroughThis PR centralizes web keyboard shortcuts: introduces a shared Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as MobileApp (resolveWebShortcuts)
participant Hook as useMobileKeyboardHandler
participant Actions as Actions Map
Note over App,Hook: Settings-derived shortcuts are resolved and passed into the hook
User->>Hook: keydown event
Hook->>Hook: matchesShortcut(shortcuts, event)?
alt Matched and action exists
Hook->>Actions: invoke(actionId)
Actions-->>Hook: action performed (e.g., select_tab, open_palette)
opt Action triggers UI/navigation
Actions->>App: update UI / call onCloseCommandPalette / dispatch websocket
end
else Matched but no handler
Hook-->>User: ignore
else No match or typing in input without modifier
Hook-->>User: ignore
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for the contribution @chr1syy — really nice work threading the desktop This branch currently has merge conflicts against |
Greptile SummaryThis PR refactors the mobile web keyboard handler from a hardcoded 5-shortcut stub into a dispatch-driven hook that reads and merges the desktop's Confidence Score: 5/5Safe to merge; all findings are P2 style issues with no impact on correctness or runtime behaviour. The core logic is sound: ref-stable event listener, correct modifier matching, proper Escape/xterm guard ordering, and memoized shortcut resolution in App.tsx. Tests cover the key scenarios and pass. The three P2 comments (missing No files require special attention for merge safety. The three P2 items are in Important Files Changed
Sequence DiagramsequenceDiagram
participant D as Desktop App
participant WS as Web Server (web-server-factory)
participant MW as Mobile Web Client
participant H as useMobileKeyboardHandler
participant A as App.tsx actions
D->>WS: settings.shortcuts changed
WS->>MW: broadcastSettingsChanged({ shortcuts })
MW->>MW: resolveWebShortcuts(userOverrides)
MW->>H: { shortcuts, actions }
Note over H: ref updated, listener stable
MW-->>H: keydown event
H->>H: xterm guard check
H->>H: Escape → onCloseCommandPalette?
H->>H: matchesShortcut(e, shortcuts[id])
H->>A: handler() for matched action ID
Reviews (1): Last reviewed commit: "feat(web): configurable hotkeys for mobi..." | Re-trigger Greptile |
| '*': '8', | ||
| '(': '9', | ||
| ')': '0', | ||
| }; |
There was a problem hiding this comment.
Missing
/ special-case diverges from desktop helper
The desktop isShortcut in useKeyboardShortcutHelpers.ts has an explicit early-return for the slash key (if (mainKey === '/' && key === '/') return true;) before all the other special-case checks. The mobile matchesShortcut omits it. The slash key falls through to the general return key === mainKey at the bottom and works in practice, but the comment above the function says "Kept in sync manually — update both if matching rules change" — the functions already diverge from day one. If someone later adds a shortcut whose mainKey is '/' the behavior may silently differ between desktop and mobile.
| audioFeedbackEnabled: settingsStore.get('audioFeedbackEnabled', false) as boolean, | ||
| colorBlindMode: settingsStore.get('colorBlindMode', 'false') as string, | ||
| conductorProfile: settingsStore.get('conductorProfile', '') as string, | ||
| shortcuts: settingsStore.get('shortcuts', {}) as Record< | ||
| string, | ||
| import('../../shared/shortcut-types').Shortcut | ||
| >, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Inline
import() type instead of a top-level import
Using an inline import('../../shared/shortcut-types').Shortcut type reference inside an as cast is non-idiomatic TypeScript and harder to read/maintain. The same pattern appears again at line 873–881. Since Shortcut is re-exported from ../../shared/shortcut-types (and already pulled into types.ts), a single top-level import would be cleaner:
import type { Shortcut } from '../../shared/shortcut-types';Then both as Record<string, Shortcut> casts can drop the inline import path entirely.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| */ | ||
|
|
||
| import type { Shortcut } from '../../shared/shortcut-types'; | ||
| import { DEFAULT_SHORTCUTS } from '../../renderer/constants/shortcuts'; |
There was a problem hiding this comment.
Renderer import in a web-only constants module
The PR description explains that Shortcut was moved to src/shared/shortcut-types.ts specifically to avoid "dragging renderer DOM types into main's tsconfig", yet this file imports DEFAULT_SHORTCUTS directly from ../../renderer/constants/shortcuts. That renderer file today happens to be a pure data constant (import type { Shortcut } only), but this cross-layer import is architecturally inconsistent with the stated goal. If DEFAULT_SHORTCUTS (or any re-export chain it picks up) ever gains a renderer-side dependency, the web bundle and main-process tsconfig will silently regress. Consider co-locating DEFAULT_SHORTCUTS in src/shared/ (or duplicating the web-relevant subset there) to keep the boundary clean.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/ipc/handlers/autorun.ts (1)
116-151:⚠️ Potential issue | 🟠 MajorDon’t include symlink targets outside the Auto Run folder.
The new scanner follows symlinked files/directories, but later
readDoc/writeDoconly validate the lexical path. A symlinked.mdunder the playbooks folder can therefore point outside the folder and still be read/written by Auto Run. Skip symlinks here, or add canonical realpath containment before listing them.Safer default: skip symlinked Auto Run docs/directories
- for (const { entry, isDirectory, isFile } of sortedEntries) { + for (const { entry, isDirectory, isFile, isSymlink } of sortedEntries) { const entryRelativePath = relativePath ? `${relativePath}/${entry.name}` : entry.name; + if (isSymlink) { + continue; + } + if (isDirectory) { @@ - for (const { entry, isDirectory, isFile } of sortedEntries) { + for (const { entry, isDirectory, isFile } of sortedEntries) { const entryRelativePath = relativePath ? `${relativePath}/${entry.name}` : entry.name; + if (entry.isSymlink) { + continue; + } + if (isDirectory) { - // Avoid infinite recursion on remote symlink cycles. - // We currently don't have remote realpath canonicalization in remote-fs, - // so skip descending into symlinked directories. - if (entry.isSymlink) { - continue; - } - @@ - if (!isDirectory && entry.isSymbolicLink()) { + if (!isDirectory && entry.isSymbolicLink()) { + continue; + }Also applies to: 213-260, 333-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/autorun.ts` around lines 116 - 151, The scanner currently follows symlinked entries (via resolveLocalEntryType/scanDirectory) which allows .md files outside the Auto Run base to be included; update scanDirectory to skip symlinks or enforce canonical containment by calling fs.realpath on each entry and comparing the resolved path against the Auto Run base realpath (use visitedRealPaths and the same realpath check wherever entries are processed — e.g., in the resolvedEntries mapping and the folder recursion block inside scanDirectory) and only include entries whose realpath startsWith the Auto Run base realpath; ensure you reference resolveLocalEntryType, scanDirectory, visitedRealPaths, and nodes when implementing this containment check so symlink targets outside the Auto Run folder are excluded before adding to nodes or recursing.src/renderer/hooks/batch/useAutoRunDocumentLoader.ts (1)
186-252:⚠️ Potential issue | 🟡 MinorCapture refresh failures from detached watcher callbacks.
refreshAutoRunData()can reject from IPC/file operations; in both remote polling and local file-change callbacks that becomes a detached unhandled rejection. Treat refresh failures as recoverable, capture them with context, and keep polling/watching.Suggested fix
import { useEffect, useCallback, useRef } from 'react'; import { useSessionStore, selectActiveSession } from '../../stores/sessionStore'; import { useBatchStore } from '../../stores/batchStore'; import { countMarkdownTasks } from './batchUtils'; +import { captureException } from '../../utils/sentry'; @@ const runRemotePoll = async () => { if (disposed || isRefreshing) return; isRefreshing = true; try { await refreshAutoRunData(); + } catch (error) { + captureException(error instanceof Error ? error : new Error(String(error)), { + extra: { + context: 'useAutoRunDocumentLoader: remote Auto Run refresh failed', + folderPath, + sshRemoteId, + }, + }); } finally { isRefreshing = false; if (!disposed) { @@ unsubscribe = window.maestro.autorun.onFileChanged(async (data) => { if (disposed) return; if (data.folderPath !== folderPath) return; - await refreshAutoRunData(); + try { + await refreshAutoRunData(); + } catch (error) { + captureException(error instanceof Error ? error : new Error(String(error)), { + extra: { + context: 'useAutoRunDocumentLoader: local Auto Run refresh failed', + folderPath, + filename: data.filename, + }, + }); + } });As per coding guidelines, “Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useAutoRunDocumentLoader.ts` around lines 186 - 252, refreshAutoRunData can throw and those throws become detached rejections in the remote poll loop (runRemotePoll) and the local onFileChanged callback; wrap each await refreshAutoRunData() call (inside runRemotePoll and inside the onFileChanged handler) in a try/catch that captures and logs the error with context (folderPath, sshRemoteId and selectedFile when available) and does not rethrow so polling/watching continues; ensure the catch reports the error to your app/error service (e.g., existing logger/Sentry wrapper) and then lets the loop/timer or watcher continue as before.
🧹 Nitpick comments (2)
src/main/agents/capabilities.ts (1)
263-263: Enabling wizard for factory-droid looks consistent.factory-droid already has
supportsStreamJsonInput: trueandsupportsThinkingDisplay: true, so enablingsupportsWizardaligns with the other wizard-capable agents (claude-code, codex, opencode). Note:src/__tests__/renderer/hooks/useInlineWizard.test.tsmockshasCapabilityCachedwith a hardcoded list['claude-code', 'codex', 'opencode']— consider updating that mock to includefactory-droidso wizard behavior is exercised in renderer tests too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/agents/capabilities.ts` at line 263, The renderer tests mock for hasCapabilityCached in useInlineWizard.test.ts currently lists wizard-capable agents ['claude-code', 'codex', 'opencode'] and thus won’t exercise factory-droid; update that mock to include 'factory-droid' (i.e., ['claude-code','codex','opencode','factory-droid']) so the test covers the newly enabled supportsWizard capability, and run the test to ensure no other assertions or snapshots need adjustment.src/main/web-server/web-server-factory.ts (1)
822-904: Optional: deduplicate the WebSettings snapshot used for initial read and broadcast.The settings object constructed at lines 825-840 and 865-880 is now an 11-field verbatim duplicate. Extracting a local
buildWebSettings()helper would prevent future fields (likeshortcutshere) from having to be added in two places and drifting.♻️ Sketch
const buildWebSettings = (): WebSettings => ({ theme: settingsStore.get('activeThemeId', 'dracula') as string, // ...all other fields... shortcuts: settingsStore.get('shortcuts', {}) as Record<string, Shortcut>, });Then both sites become
const settings = buildWebSettings();.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/web-server/web-server-factory.ts` around lines 822 - 904, The duplicated construction of the WebSettings object in server.setGetSettingsCallback and inside the success branch of server.setSetSettingCallback should be extracted into a single helper (e.g., buildWebSettings) that reads from settingsStore and returns the WebSettings shape; replace both inline object literals with const settings = buildWebSettings() and use settings for broadcasting and returning so future fields like shortcuts are maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/agent/useAgentExecution.ts`:
- Around line 40-41: The hard timeout constant BATCH_HARD_TIMEOUT_MS should be
derived from the configured inactivity timeout (not a fixed 60 minutes) so set
it to the inactivity config value (e.g., use the same value used for
BATCH_WATCHDOG_CHECK_MS/inactivity settings) and update any watchdog logic that
uses BATCH_HARD_TIMEOUT_MS; in the code paths inside useAgentExecution where
kill() is invoked (and in the related watchdog/cleanup logic around lines
referenced), await the kill() promise and do not swallow errors — catch only
expected recoverable errors explicitly and rethrow or log unexpected ones so
they bubble to Sentry, and ensure you only resolve/allow the next task to start
after kill() has completed (or an explicit handled timeout) so the stalled
process is truly terminated before proceeding.
In `@src/renderer/hooks/batch/useBatchProcessor.ts`:
- Line 2020: The current call to Promise.allSettled(...) around
batchProcessIds.map(id => window.maestro.process.kill(id)) swallows failures;
change useBatchProcessor.ts so after awaiting Promise.allSettled on
window.maestro.process.kill for each id you inspect the returned results array,
filter entries with status === 'rejected', and if any exist either log/report
them (or rethrow a combined Error) before clearing/resetting batch state; only
proceed to reset state when all kill promises succeeded, and reference the
batchProcessIds variable and the window.maestro.process.kill calls in your
handling logic.
- Around line 1087-1088: The try/catch inside useBatchProcessor that currently
does "catch { /* Ignore polling errors */ }" silently swallows all errors;
change it to catch the error object (e.g., catch (err)) and explicitly handle
known transient/read-race cases (check err.code for ENOENT, EBUSY, EAGAIN or use
a helper like isTransientReadError), while for any other unexpected errors call
the global error reporter (Sentry.captureException or processLogger.error with
context including function name useBatchProcessor and the file/agent id) or
rethrow so it bubbles up; ensure you reference the same catch block
(polling/read failure handling) and add minimal tests or logging to confirm
non-transient failures are reported.
- Around line 1022-1034: The progress polling cleanup (stopProgressPolling,
progressPollTimeout) is scoped inside the task loop so the hook-level unmount
cleanup can't cancel an outstanding timeout; register the polling teardown with
the hook lifecycle by exposing or adding stopProgressPolling to the hook's
cleanup handlers (e.g., push stopProgressPolling or a wrapper that clears
progressPollTimeout into the hook-level cleanup registry used by
useBatchProcessor) and ensure the hook's useEffect cleanup calls it; also remove
the registered handler from the registry inside both normal and error paths
after you call stopProgressPolling() so no dangling references remain and
onUpdateSession cannot be invoked after unmount.
- Around line 1947-1950: Replace the console.assert in killBatchRun with a
thrown error to enforce the invariant: check that sessionId does not include
'-batch-' and if it does, throw a descriptive Error (e.g., include the sessionId
and the message '[BatchProcessor:killBatchRun] sessionId must not contain
"-batch-"') so the failure propagates to error boundaries/Sentry; update any
tests or callers of killBatchRun to expect an exception instead of a silent
assertion failure.
In `@src/renderer/stores/settingsStore.ts`:
- Around line 1056-1060: The stored setting is read directly into state
elsewhere and bypasses the setter's 1..600 clamp, so ensure any
load/initialization that reads
window.maestro.settings.get('autoRunInactivityTimeoutMin') applies the same
clamp before calling set; reuse the exact clamp logic used in
setAutoRunInactivityTimeoutMin (const clamped = Math.max(1, Math.min(600,
Math.round(value)))) and write that clamped value to state and to
window.maestro.settings where appropriate (i.e., wherever the stored value is
applied during initialization or reload), so stale/manual/external values cannot
set an out-of-range autoRunInactivityTimeoutMin.
In `@src/web/mobile/App.tsx`:
- Around line 2170-2172: The goToAutoRun action currently calls
handleOpenAutoRunPanel (opening the full-screen Auto Run panel) which is
inconsistent with goToFiles/goToHistory that open right-drawer tabs; change
goToAutoRun to call handleOpenRightDrawer with the Auto Run tab key (e.g.
replace handleOpenAutoRunPanel() with handleOpenRightDrawer('autoRun')) so the
shortcut opens the same right-drawer Auto Run tab as the command-palette
navigation.
---
Outside diff comments:
In `@src/main/ipc/handlers/autorun.ts`:
- Around line 116-151: The scanner currently follows symlinked entries (via
resolveLocalEntryType/scanDirectory) which allows .md files outside the Auto Run
base to be included; update scanDirectory to skip symlinks or enforce canonical
containment by calling fs.realpath on each entry and comparing the resolved path
against the Auto Run base realpath (use visitedRealPaths and the same realpath
check wherever entries are processed — e.g., in the resolvedEntries mapping and
the folder recursion block inside scanDirectory) and only include entries whose
realpath startsWith the Auto Run base realpath; ensure you reference
resolveLocalEntryType, scanDirectory, visitedRealPaths, and nodes when
implementing this containment check so symlink targets outside the Auto Run
folder are excluded before adding to nodes or recursing.
In `@src/renderer/hooks/batch/useAutoRunDocumentLoader.ts`:
- Around line 186-252: refreshAutoRunData can throw and those throws become
detached rejections in the remote poll loop (runRemotePoll) and the local
onFileChanged callback; wrap each await refreshAutoRunData() call (inside
runRemotePoll and inside the onFileChanged handler) in a try/catch that captures
and logs the error with context (folderPath, sshRemoteId and selectedFile when
available) and does not rethrow so polling/watching continues; ensure the catch
reports the error to your app/error service (e.g., existing logger/Sentry
wrapper) and then lets the loop/timer or watcher continue as before.
---
Nitpick comments:
In `@src/main/agents/capabilities.ts`:
- Line 263: The renderer tests mock for hasCapabilityCached in
useInlineWizard.test.ts currently lists wizard-capable agents ['claude-code',
'codex', 'opencode'] and thus won’t exercise factory-droid; update that mock to
include 'factory-droid' (i.e.,
['claude-code','codex','opencode','factory-droid']) so the test covers the newly
enabled supportsWizard capability, and run the test to ensure no other
assertions or snapshots need adjustment.
In `@src/main/web-server/web-server-factory.ts`:
- Around line 822-904: The duplicated construction of the WebSettings object in
server.setGetSettingsCallback and inside the success branch of
server.setSetSettingCallback should be extracted into a single helper (e.g.,
buildWebSettings) that reads from settingsStore and returns the WebSettings
shape; replace both inline object literals with const settings =
buildWebSettings() and use settings for broadcasting and returning so future
fields like shortcuts are maintained in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ea38f1a-aee2-4fdc-9d48-d4acb7656686
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
src/__tests__/main/agents/capabilities.test.tssrc/__tests__/renderer/hooks/useAgentExecution.test.tssrc/__tests__/renderer/hooks/useBatchHandlers.test.tssrc/__tests__/web/hooks/useMobileKeyboardHandler.test.tssrc/main/agents/capabilities.tssrc/main/ipc/handlers/autorun.tssrc/main/ipc/handlers/filesystem.tssrc/main/web-server/managers/CallbackRegistry.tssrc/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/renderer/components/CuePipelineEditor/utils/pipelineToYaml.tssrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/global.d.tssrc/renderer/hooks/agent/useAgentExecution.tssrc/renderer/hooks/batch/batchUtils.tssrc/renderer/hooks/batch/useAutoRunDocumentLoader.tssrc/renderer/hooks/batch/useAutoRunHandlers.tssrc/renderer/hooks/batch/useAutoRunMarkdown.tssrc/renderer/hooks/batch/useBatchHandlers.tssrc/renderer/hooks/batch/useBatchProcessor.tssrc/renderer/hooks/batch/useDocumentProcessor.tssrc/renderer/hooks/settings/useSettings.tssrc/renderer/stores/batchStore.tssrc/renderer/stores/settingsStore.tssrc/renderer/types/index.tssrc/shared/settingsMetadata.tssrc/shared/shortcut-types.tssrc/web/constants/webShortcuts.tssrc/web/hooks/index.tssrc/web/hooks/useMobileKeyboardHandler.tssrc/web/hooks/useWebSocket.tssrc/web/mobile/App.tsx
💤 Files with no reviewable changes (2)
- src/renderer/components/CuePipelineEditor/utils/pipelineToYaml.ts
- src/renderer/global.d.ts
| const BATCH_HARD_TIMEOUT_MS = 60 * 60 * 1000; // 60 minutes wall clock per task | ||
| const BATCH_WATCHDOG_CHECK_MS = 15 * 1000; // Check every 15 seconds |
There was a problem hiding this comment.
Align the hard timeout with the configured inactivity timeout and don’t swallow kill failures.
With a 120-minute inactivity setting, the 60-minute hard timeout still wins. Also, resolving immediately after kill() is fired can allow the next task to start before the stalled process has actually been handled.
Suggested fix
import { estimateContextUsage } from '../../utils/contextUsage';
import { useSettingsStore } from '../../stores/settingsStore';
+import { captureException } from '../../utils/sentry';
import { logger } from '../../utils/logger';
@@
if (isBatchProcess) {
const inactivityTimeoutMin = useSettingsStore.getState().autoRunInactivityTimeoutMin;
const inactivityTimeoutMs = inactivityTimeoutMin * 60 * 1000;
+ const hardTimeoutMs = Math.max(
+ BATCH_HARD_TIMEOUT_MS,
+ inactivityTimeoutMs + BATCH_WATCHDOG_CHECK_MS
+ );
+
+ const killAndResolve = (result: AgentSpawnResult) => {
+ if (settled) return;
+ void window.maestro.process
+ .kill(targetSessionId)
+ .catch((killError) => {
+ captureException(
+ killError instanceof Error ? killError : new Error(String(killError)),
+ {
+ extra: {
+ context: 'spawnAgentForSession: watchdog kill failed',
+ targetSessionId,
+ errorKind: result.errorKind,
+ },
+ }
+ );
+ })
+ .finally(() => resolveOnce(result));
+ };
inactivityTimer = setInterval(() => {
if (settled) return;
if (Date.now() - lastOutputAt <= inactivityTimeoutMs) return;
- window.maestro.process.kill(targetSessionId).catch(() => {});
- resolveOnce({
+ killAndResolve({
success: false,
error: `Agent task stalled: no output for ${inactivityTimeoutMin} minutes`,
errorKind: 'watchdog-stalled',
@@
hardTimeoutTimer = setTimeout(() => {
if (settled) return;
- window.maestro.process.kill(targetSessionId).catch(() => {});
- resolveOnce({
+ killAndResolve({
success: false,
- error: `Agent task timed out after ${Math.floor(BATCH_HARD_TIMEOUT_MS / 60000)} minutes`,
+ error: `Agent task timed out after ${Math.floor(hardTimeoutMs / 60000)} minutes`,
errorKind: 'watchdog-timeout',
response: responseText,
agentSessionId,
usageStats: taskUsageStats,
});
- }, BATCH_HARD_TIMEOUT_MS);
+ }, hardTimeoutMs);
}As per coding guidelines, “Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly.”
Also applies to: 490-521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/agent/useAgentExecution.ts` around lines 40 - 41, The hard
timeout constant BATCH_HARD_TIMEOUT_MS should be derived from the configured
inactivity timeout (not a fixed 60 minutes) so set it to the inactivity config
value (e.g., use the same value used for BATCH_WATCHDOG_CHECK_MS/inactivity
settings) and update any watchdog logic that uses BATCH_HARD_TIMEOUT_MS; in the
code paths inside useAgentExecution where kill() is invoked (and in the related
watchdog/cleanup logic around lines referenced), await the kill() promise and do
not swallow errors — catch only expected recoverable errors explicitly and
rethrow or log unexpected ones so they bubble to Sentry, and ensure you only
resolve/allow the next task to start after kill() has completed (or an explicit
handled timeout) so the stalled process is truly terminated before proceeding.
| } catch { | ||
| // Ignore polling errors — agent may be modifying file |
There was a problem hiding this comment.
Don’t silently swallow polling failures.
This catches every polling/read failure and hides real IPC, permission, or remote-read regressions. Handle known transient races explicitly, and report unexpected failures with context.
As per coding guidelines, “Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 1087 - 1088, The
try/catch inside useBatchProcessor that currently does "catch { /* Ignore
polling errors */ }" silently swallows all errors; change it to catch the error
object (e.g., catch (err)) and explicitly handle known transient/read-race cases
(check err.code for ENOENT, EBUSY, EAGAIN or use a helper like
isTransientReadError), while for any other unexpected errors call the global
error reporter (Sentry.captureException or processLogger.error with context
including function name useBatchProcessor and the file/agent id) or rethrow so
it bubbles up; ensure you reference the same catch block (polling/read failure
handling) and add minimal tests or logging to confirm non-transient failures are
reported.
| console.assert( | ||
| !sessionId.includes('-batch-'), | ||
| '[BatchProcessor:killBatchRun] sessionId must not contain "-batch-"' | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify killBatchRun callers only pass root Auto Run session IDs, not per-task "-batch-" child IDs.
rg -n -C4 '\bkillBatchRun\b' -t ts -t tsxRepository: RunMaestro/Maestro
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for killBatchRun calls using correct ripgrep syntax
rg -n -C4 'killBatchRun' -t tsRepository: RunMaestro/Maestro
Length of output: 13162
🏁 Script executed:
#!/bin/bash
# Check if useBatchProcessor.ts imports Sentry and how errors are currently handled
head -50 src/renderer/hooks/batch/useBatchProcessor.tsRepository: RunMaestro/Maestro
Length of output: 2013
🏁 Script executed:
#!/bin/bash
# Look for the specific line 1947-1950 context in useBatchProcessor.ts
sed -n '1940,1960p' src/renderer/hooks/batch/useBatchProcessor.tsRepository: RunMaestro/Maestro
Length of output: 1100
🏁 Script executed:
#!/bin/bash
# Check if there are any scenarios where killBatchRun is called with "-batch-" child IDs
rg -n 'batch-' src/renderer/hooks/batch/ -t ts | head -30Repository: RunMaestro/Maestro
Length of output: 668
🏁 Script executed:
#!/bin/bash
# Verify that sessionId format is consistent - root IDs don't contain "-batch-"
# Check where "-batch-" suffix is added (should be for child task IDs, not passed to killBatchRun)
rg -n '"-batch-"' src/renderer/ -t ts | grep -E '(sessionId|child|format)' | head -20Repository: RunMaestro/Maestro
Length of output: 185
Replace console.assert with a thrown error for proper error handling.
console.assert is unreliable in renderer/browser contexts; execution continues even when the condition fails. This invariant is critical for kill scoping—killBatchRun must receive only root session IDs (not child batch process IDs like ${sessionId}-batch-${timestamp}). Throwing an error ensures proper error propagation to Sentry via error boundaries.
Proposed fix
- console.assert(
- !sessionId.includes('-batch-'),
- '[BatchProcessor:killBatchRun] sessionId must not contain "-batch-"'
- );
+ if (sessionId.includes('-batch-')) {
+ throw new Error('[BatchProcessor:killBatchRun] sessionId must not contain "-batch-"');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.assert( | |
| !sessionId.includes('-batch-'), | |
| '[BatchProcessor:killBatchRun] sessionId must not contain "-batch-"' | |
| ); | |
| if (sessionId.includes('-batch-')) { | |
| throw new Error('[BatchProcessor:killBatchRun] sessionId must not contain "-batch-"'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 1947 - 1950,
Replace the console.assert in killBatchRun with a thrown error to enforce the
invariant: check that sessionId does not include '-batch-' and if it does, throw
a descriptive Error (e.g., include the sessionId and the message
'[BatchProcessor:killBatchRun] sessionId must not contain "-batch-"') so the
failure propagates to error boundaries/Sentry; update any tests or callers of
killBatchRun to expect an exception instead of a silent assertion failure.
| batchProcessIds.push(sessionId); | ||
| } | ||
|
|
||
| await Promise.allSettled(batchProcessIds.map((id) => window.maestro.process.kill(id))); |
There was a problem hiding this comment.
Inspect rejected kill attempts before resetting state.
Promise.allSettled prevents rejected process.kill calls from reaching the catch block, so the UI can clear the batch while one or more agent processes are still alive.
Proposed failure handling
- await Promise.allSettled(batchProcessIds.map((id) => window.maestro.process.kill(id)));
+ const killResults = await Promise.allSettled(
+ batchProcessIds.map((id) => window.maestro.process.kill(id))
+ );
+ const failedProcessIds = killResults.flatMap((result, index) =>
+ result.status === 'rejected' ? [batchProcessIds[index]] : []
+ );
+ if (failedProcessIds.length > 0) {
+ throw new Error(
+ `Failed to kill Auto Run process(es): ${failedProcessIds.join(', ')}`
+ );
+ }As per coding guidelines, “Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await Promise.allSettled(batchProcessIds.map((id) => window.maestro.process.kill(id))); | |
| const killResults = await Promise.allSettled( | |
| batchProcessIds.map((id) => window.maestro.process.kill(id)) | |
| ); | |
| const failedProcessIds = killResults.flatMap((result, index) => | |
| result.status === 'rejected' ? [batchProcessIds[index]] : [] | |
| ); | |
| if (failedProcessIds.length > 0) { | |
| throw new Error( | |
| `Failed to kill Auto Run process(es): ${failedProcessIds.join(', ')}` | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useBatchProcessor.ts` at line 2020, The current call
to Promise.allSettled(...) around batchProcessIds.map(id =>
window.maestro.process.kill(id)) swallows failures; change useBatchProcessor.ts
so after awaiting Promise.allSettled on window.maestro.process.kill for each id
you inspect the returned results array, filter entries with status ===
'rejected', and if any exist either log/report them (or rethrow a combined
Error) before clearing/resetting batch state; only proceed to reset state when
all kill promises succeeded, and reference the batchProcessIds variable and the
window.maestro.process.kill calls in your handling logic.
| setAutoRunInactivityTimeoutMin: (value) => { | ||
| const clamped = Math.max(1, Math.min(600, Math.round(value))); | ||
| set({ autoRunInactivityTimeoutMin: clamped }); | ||
| window.maestro.settings.set('autoRunInactivityTimeoutMin', clamped); | ||
| }, |
There was a problem hiding this comment.
Clamp the timeout on load too.
Line 2044 bypasses the setter’s 1..600 clamp, so a stale/manual/external setting can put an invalid timeout into state and break the Auto Run watchdog.
Suggested fix
+const clampAutoRunInactivityTimeoutMin = (value: number): number =>
+ Math.max(1, Math.min(600, Math.round(value)));
+
setAutoRunInactivityTimeoutMin: (value) => {
- const clamped = Math.max(1, Math.min(600, Math.round(value)));
+ const clamped = clampAutoRunInactivityTimeoutMin(value);
set({ autoRunInactivityTimeoutMin: clamped });
window.maestro.settings.set('autoRunInactivityTimeoutMin', clamped);
},
@@
if (allSettings['autoRunInactivityTimeoutMin'] !== undefined)
- patch.autoRunInactivityTimeoutMin = allSettings['autoRunInactivityTimeoutMin'] as number;
+ patch.autoRunInactivityTimeoutMin = clampAutoRunInactivityTimeoutMin(
+ allSettings['autoRunInactivityTimeoutMin'] as number
+ );Also applies to: 2043-2044
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/stores/settingsStore.ts` around lines 1056 - 1060, The stored
setting is read directly into state elsewhere and bypasses the setter's 1..600
clamp, so ensure any load/initialization that reads
window.maestro.settings.get('autoRunInactivityTimeoutMin') applies the same
clamp before calling set; reuse the exact clamp logic used in
setAutoRunInactivityTimeoutMin (const clamped = Math.max(1, Math.min(600,
Math.round(value)))) and write that clamped value to state and to
window.maestro.settings where appropriate (i.e., wherever the stored value is
applied during initialization or reload), so stale/manual/external values cannot
set an out-of-range autoRunInactivityTimeoutMin.
…tings Refactors the mobile keyboard handler from a hardcoded 5-shortcut stub into a dispatch-driven hook that reads the desktop's `shortcuts` setting over WebSocket. 16 web-supported actions are wired (palette, mode toggle, prev/next tab, prev/next agent, new agent, settings, files/history/auto-run panels, all sessions, usage dashboard, Cue, group chat, interrupt). Behavior change: Cmd+[/] now cycles agents (matches desktop); tabs move to Cmd+Shift+[/]. - Add `shortcuts` to WebSettings + getSettings/broadcast paths - Move `Shortcut` interface to src/shared/shortcut-types.ts - Inline matcher in useMobileKeyboardHandler mirrors desktop's helper - Curated WEB_SHORTCUT_IDS in src/web/constants/webShortcuts.ts Note: desktop-side shortcut edits require a web reconnect to take effect (matches existing WebSettings broadcast behavior). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add `/` special-case to mobile shortcut matcher to stay in sync with the desktop helper at src/renderer/hooks/keyboard/useKeyboardShortcutHelpers.ts - Replace inline `import()` type casts in web-server-factory.ts with a top-level `Shortcut` import for readability - Route `goToAutoRun` shortcut to the right-drawer Auto Run tab so it's consistent with `goToFiles` and `goToHistory` (not the full-screen panel) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
259c686 to
9bd0dc6
Compare
|
Rebased onto Fixed:
Acknowledged, not fixed (architectural tradeoff):
Type check, ESLint, Prettier clean. In-scope tests: 6/6 + 321 web-server tests pass (352 total). |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/web/hooks/useMobileKeyboardHandler.ts`:
- Around line 149-174: The global shortcut handler fires configurable single-key
shortcuts while the user types; add an editable-target guard in
useMobileKeyboardHandler before iterating actions so we skip shortcuts when
focus is inside an editable element. Implement an isEditableElement check (using
document.activeElement or event.target) that returns true for input/textarea or
elements with contentEditable=true but false for xterm (reuse isXtermElement),
and if it returns true && activeSession?.inputMode !== 'terminal' then return
early; keep matchesShortcut, actions and shortcuts logic unchanged otherwise.
- Around line 67-109: The matchesShortcut function can fire for empty or
modifier-only overrides; update matchesShortcut to guard early: if sc.keys is
empty or the derived mainKey is undefined or is one of the modifier names
('meta','ctrl','command','shift','alt'), return false before any key comparisons
or the shiftNumberMap lookup. Specifically, in matchesShortcut, validate sc.keys
and mainKey (the last item in keys) and only proceed to compare meta/shift/alt,
arrow/symbol checks, and the shiftNumberMap lookup when mainKey exists and is
not a pure modifier to avoid matching ordinary typing when shortcuts are
cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c734631-aa14-4606-b633-a8fc928e8930
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
src/__tests__/web/hooks/useMobileKeyboardHandler.test.tssrc/main/web-server/managers/CallbackRegistry.tssrc/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/renderer/types/index.tssrc/shared/shortcut-types.tssrc/web/constants/webShortcuts.tssrc/web/hooks/index.tssrc/web/hooks/useMobileKeyboardHandler.tssrc/web/hooks/useWebSocket.tssrc/web/mobile/App.tsx
✅ Files skipped from review due to trivial changes (3)
- src/web/hooks/index.ts
- src/renderer/types/index.ts
- src/shared/shortcut-types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/web-server/managers/CallbackRegistry.ts
- src/web/constants/webShortcuts.ts
- src/tests/web/hooks/useMobileKeyboardHandler.test.ts
Addresses CodeRabbit review on PR RunMaestro#882 and the failing CI tests in src/__tests__/web/mobile/App.test.tsx. - matchesShortcut: return false when sc.keys is empty or the trailing key is a pure modifier (meta/ctrl/command/shift/alt). Prevents a cleared/malformed override from firing on ordinary typing. - Mobile keyboard handler: skip dispatch when focus is inside an editable element (input/textarea/select/contenteditable) and no modifier key is pressed. Modifier shortcuts (Cmd/Ctrl/Alt) still fire from inputs so palette / mode toggle work as expected. - App.test.tsx: update the 3 tab-navigation tests to fire the new Cmd+Shift+[/] binding (Cmd+[/] now cycles agents per desktop). - New unit tests cover the editable-target guard and modifier-only shortcut def cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed Fixed (both CodeRabbit major):
CI test failure fixed:
New tests in
Full scoped suite (450 tests across |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/web/hooks/useMobileKeyboardHandler.ts (1)
35-39: NarrowMobileKeyboardSessionto what the hook actually reads.The hook only uses
activeSession?.inputModefor xterm isolation; tab state now lives behindactions. KeepingaiTabs/activeTabIdhere makes the public hook contract look more coupled than it is.♻️ Proposed cleanup
export type MobileKeyboardSession = { inputMode?: string; - aiTabs?: AITabData[]; - activeTabId?: string; };Then remove the now-unused
AITabDataimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/hooks/useMobileKeyboardHandler.ts` around lines 35 - 39, The MobileKeyboardSession type is broader than the hook needs; update the type to only include the field the hook reads (inputMode) and remove aiTabs and activeTabId from MobileKeyboardSession, then remove the now-unused AITabData import; locate the MobileKeyboardSession declaration and the useMobileKeyboardHandler hook to change the exported type to: { inputMode?: string } and delete any imports or references to AITabData that become unused.src/__tests__/web/hooks/useMobileKeyboardHandler.test.ts (1)
274-293: Add the modifier-only case promised by the test name.This currently only verifies
keys: []. Add a second assertion for something likekeys: ['Meta']so the modifier-only guard stays covered too.🧪 Proposed test expansion
it('does not match an empty or modifier-only shortcut definition', () => { const newInstance = vi.fn(); const shortcuts = { newInstance: { id: 'newInstance', label: 'New Agent', keys: [] }, }; renderHook(() => useMobileKeyboardHandler({ shortcuts, activeSession: null, actions: { newInstance }, }) ); act(() => { document.dispatchEvent(new KeyboardEvent('keydown', { key: 'n', cancelable: true })); }); expect(newInstance).not.toHaveBeenCalled(); + + const modifierOnlyShortcuts = { + newInstance: { id: 'newInstance', label: 'New Agent', keys: ['Meta'] }, + }; + + renderHook(() => + useMobileKeyboardHandler({ + shortcuts: modifierOnlyShortcuts, + activeSession: null, + actions: { newInstance }, + }) + ); + + act(() => { + document.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Meta', metaKey: true, cancelable: true }) + ); + }); + + expect(newInstance).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/web/hooks/useMobileKeyboardHandler.test.ts` around lines 274 - 293, Update the test case "does not match an empty or modifier-only shortcut definition" to also cover the modifier-only shortcut: create a second shortcut entry (e.g., shortcuts.modOnly = { id: 'modOnly', label: 'Modifier Only', keys: ['Meta'] }) and render useMobileKeyboardHandler with that shortcut and the same spy action; then dispatch a non-modifier KeyboardEvent (like key: 'n') and assert that the mod-only action (the spy) is not called. Ensure the test still includes the existing empty-keys case and references the same newInstance spy and useMobileKeyboardHandler invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/web/hooks/useMobileKeyboardHandler.test.ts`:
- Around line 274-293: Update the test case "does not match an empty or
modifier-only shortcut definition" to also cover the modifier-only shortcut:
create a second shortcut entry (e.g., shortcuts.modOnly = { id: 'modOnly',
label: 'Modifier Only', keys: ['Meta'] }) and render useMobileKeyboardHandler
with that shortcut and the same spy action; then dispatch a non-modifier
KeyboardEvent (like key: 'n') and assert that the mod-only action (the spy) is
not called. Ensure the test still includes the existing empty-keys case and
references the same newInstance spy and useMobileKeyboardHandler invocation.
In `@src/web/hooks/useMobileKeyboardHandler.ts`:
- Around line 35-39: The MobileKeyboardSession type is broader than the hook
needs; update the type to only include the field the hook reads (inputMode) and
remove aiTabs and activeTabId from MobileKeyboardSession, then remove the
now-unused AITabData import; locate the MobileKeyboardSession declaration and
the useMobileKeyboardHandler hook to change the exported type to: { inputMode?:
string } and delete any imports or references to AITabData that become unused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50f564ed-928b-4e56-a7d0-5e4c70313ef7
📒 Files selected for processing (3)
src/__tests__/web/hooks/useMobileKeyboardHandler.test.tssrc/__tests__/web/mobile/App.test.tsxsrc/web/hooks/useMobileKeyboardHandler.ts
- LeftPanel (mobile): include setShowUnreadOnly in the auto-disable
effect's deps so React's exhaustive-deps lint stays satisfied now
that the setter arrives via props.
- web-server-factory: collapse the inline `import('../../shared/...').
Shortcut` to the existing top-level type import.
- agents.test (SSH detect): tighten the call-count check to
AGENT_DEFINITIONS.length so a regression that silently skips agents
is caught instead of just dropping below the prior `> 0` threshold.
Summary
Refactors the mobile web keyboard handler from a hardcoded 5-shortcut stub into a dispatch-driven hook that reads the desktop's
shortcutssetting over WebSocket. 16 web-supported actions are wired (palette, mode toggle, prev/next tab, prev/next agent, new agent, settings, files/history/auto-run panels, all sessions, usage dashboard, Cue, group chat, interrupt).shortcutstoWebSettings+getSettings/broadcast paths so web clients receive desktop's shortcut customizations.Shortcutinterface tosrc/shared/shortcut-types.ts(re-exported fromsrc/renderer/types) so main + web can import without dragging renderer DOM types into main's tsconfig.useMobileKeyboardHandler.tsmirrors the desktop helper atsrc/renderer/hooks/keyboard/useKeyboardShortcutHelpers.ts(no React-hook import from a renderer path).WEB_SHORTCUT_IDSinsrc/web/constants/webShortcuts.tsfiltersDEFAULT_SHORTCUTSto only the action IDs the mobile UI implements.Behavior change
Cmd+[/Cmd+]Cmd+Shift+[/Cmd+Shift+]Cmd+[/Cmd+]Limitations
WebSettingsfields today.Branch state note
This branch is behind
upstream/rcby ~10 commits. The PR diff will include unrelated files because of how the branch was based on the merged pr-846 work. Mergingupstream/rcinto this branch before review will collapse the diff to just the hotkeys changes.Test plan
npm run lint) cleanuseMobileKeyboardHandlerunit tests rewritten for new API — 6/6 passweb-server/test suite passes (321/321)cyclePrev), reconnect mobile, verify new binding takes effect🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests